add a workaround for getFunctions in Thrift#979
Conversation
samikshya-db
changed the title
jayantsing-db
left a comment
jayantsing-db
left a comment
There was a problem hiding this comment.
Some suggestions inline.
| functionNamePattern = | ||
| "%"; // This is because functionName is a required parameter in thrift flow. |
There was a problem hiding this comment.
if the function name pattern is null, thrift-server automatically treats it as .*/%
There was a problem hiding this comment.
In case of thrift get function request, it is a required field. We can't get past the null error in the model itself. Hence, adding this.
| } | ||
| } catch (Exception e) { | ||
| LOGGER.error(e, "Unable to fetch functions, returning empty result set"); | ||
| LOGGER.error(e, "Unable to fetch functions with error {}, returning empty result set", e); |
There was a problem hiding this comment.
nit: e is already passed to log statement, any specific need to re-add and stringify it.
There was a problem hiding this comment.
We want to log the error too
Unable to fetch functions, returning empty result set java.lang.NullPointerException: Cannot invoke "com.databricks.jdbc.api.internal.IDatabricksStatementInternal.getStatementId()" because "parentStatement" is null
inplace of
2025-09-10 19:31:37 SEVERE com.databricks.jdbc.api.impl.DatabricksDatabaseMetaData#getFunctions - Unable to fetch functions, returning empty result set
| Statement internalStatement = connection.createStatement(); | ||
| String showFunctionsSqlCommand = | ||
| new CommandBuilder(catalog, session) | ||
| .setSchemaPattern(schemaPattern) | ||
| .setFunctionPattern(functionNamePattern) | ||
| .getSQLString(LIST_FUNCTIONS); | ||
| DatabricksResultSet rs = | ||
| (DatabricksResultSet) internalStatement.executeQuery(showFunctionsSqlCommand); | ||
| return new MetadataResultSetBuilder(connection.getConnectionContext()) | ||
| .getFunctionsResult(rs, catalog); |
There was a problem hiding this comment.
should we use iDatabrickClient#executeStatement directly as it is an internal impl detail? This may save us from creating a full statement object and then closing the statement object.
There was a problem hiding this comment.
Makes sense. Done.
## Description - This issue was uncovered as part of bug raised in #958. Although the issue was resolved as part of PR #979 - the cause is deeper than that - TCLIService.client causes state leakage in case previous server call - This PR fixes the issue. - Internal doc on root-cause and analysis: https://docs.google.com/document/d/12kciNIqy-kL1e0HOQs9JOjJlb2zWeNFl02e9kO772PE/edit?tab=t.0 ## Testing - Reset code before [this commit](fcfa625) where we fixed getFunctions, and the following code no longer throws error ``` for (int i = 0; i < 10; i++) { try { DatabaseMetaData metaData = con.getMetaData(); metaData.getFunctions("main", "testschema", null).close(); // Metadata call Statement stmt = con.createStatement(); stmt.execute("select * from samikshyachand.default.hello_table"); // SQL query stmt.close(); Thread.sleep(100); // Add a small delay } catch (Exception e) { e.printStackTrace(); } } ``` ## Additional Notes to the Reviewer <!-- Share any additional context or insights that may help the reviewer understand the changes better. This could include challenges faced, limitations, or compromises made during the development process. Also, mention any areas of the code that you would like the reviewer to focus on specifically. -->
3 participants
Description
EnableShowCommandForGetFunctionsTesting
Additional Notes to the Reviewer
NO_CHANGELOG=true